Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gccCrossStageStatic: enable dynamic libraries, rename it #238154

Merged
6 commits merged into from Jul 12, 2023
Merged

gccCrossStageStatic: enable dynamic libraries, rename it #238154

6 commits merged into from Jul 12, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jun 16, 2023

Best reviewed one commit at a time.

Motivation
  1. Fix a serious bug in our pkgsCross bootstrap: cross compiled binaries segfault on pthread_cancel(3) due to missing libgcc_s.so #240484. This is the reason for the backport request.
    a. Also: Closes libgcc_s.so is missing from glibc when cross compiling #40797

  2. Make our cross-compiler bootstrap look more like our stdenv bootstrap, as a step towards unifying them.

Summary

This PR allows gccCrossStageStatic to build dynamically-linked libraries. Since is no longer restricted to building static libraries its name is no longer appropriate, and this PR also renames it to the more-accurate gccWithoutTargetLibc.

Detailed description of changes

By default, you can't build a gcc that knows how to create dynamic libraries unless you have already built the targetPlatform libc.

Because of this, our gcc cross-compiler is built in three stages:

  1. Build a cross-compiler (gccCrossStageStatic) that can build only static libraries.

  2. Use gccCrossStageStatic to compile the targetPlatform libc.a.

  3. Use the targetPlatform libc.a to build a fully-capable cross compiler.

You might notice that this pattern looks very similar to what we do with xgcc and glibc in the stdenv bootstrap. Indeed it is! I would like to work towards getting the existing stdenv bootstrap to handle cross compilers as well. However we don't want to cripple stdenv.xgcc by taking away its ability to build dynamic libraries.

It turns out that the only thing gcc needs the targetPlatform libc for is to emit a DT_NEEDED for -lc into libgcc.so. That's it! And since we don't use gccCrossStageStatic to build anything other than libc, it's safe to omit the DT_NEEDED because that libgcc will never be loaded by anything other than libc. So libc will already be in the process's address space.

Other people have noticed this; crosstool-ng has been using this approach for a very long time:

https://github.com/crosstool-ng/crosstool-ng/blob/36ad0b17a732aaffe4701d5d8d410d6e3e3abba9/scripts/build/cc/gcc.sh#L638-L640

Closes #240484

Closes #240435

Things done
  • Built on platform(s)
    • x86_64-linux
    • powerpc64le-linux
    • aarch64-linux (cross from x86_64-linux)
    • mips64el-linuxabi64 (cross from x86_64-linux)
    • mips64el-linuxabin32 (cross from x86_64-linux)
    • arm-eabi gcc6 (cross from x86_64-linux)
  • Built to test gcc versions
    • pkgsCross.aarch64-multiplatform
    • .pkgsBuildTarget.gcc48
    • .pkgsBuildTarget.gcc49
    • .pkgsBuildTarget.gcc6
    • .pkgsBuildTarget.gcc7
    • .pkgsBuildTarget.gcc8
    • .pkgsBuildTarget.gcc9
    • .pkgsBuildTarget.gcc10
    • .pkgsBuildTarget.gcc11
    • .pkgsBuildTarget.gcc12
    • .pkgsBuildTarget.gcc13
  • Tested complete userspace rebuild on platform(s):
    • powerpc64le-linux
    • aarch64-linux (cross from x86_64-linux)
    • mips64el-linuxabi64+mips64el-linuxabin32 (mixed) (cross from x86_64-linux)

@ghost
Copy link
Author

ghost commented Jun 16, 2023

I'm testing builds on a pretty large matrix of target platforms and gcc versions.

In spite of this, I have the nagging feeling that this is going to break some obscure gcc version on an infrequently-used platform. I've tried to keep this PR master-eligible (i.e. no mass-rebuilds) so that if that kind of breakage does happen,

  1. People will be able to find the cause (this PR) without having to git bisect across a merge of staging into master
  2. The fix PR can go to master instead of staging.

@ghost

This comment was marked as outdated.

@ghost ghost marked this pull request as ready for review June 18, 2023 03:32
@ghost ghost requested review from matthewbauer and Ericson2314 as code owners June 18, 2023 03:32
@ghost
Copy link
Author

ghost commented Jun 19, 2023

Everything tests fine, rebooted the laptop with a complete system (bootloader, kernel, userspace) built this way.

However the change made in gcc commit e50084fa44cb68c447efc11e53ec16bf09a578c0 needs to be made to libstdc++ as well; I will send a patch to do that upstream and include it here.

Edit: what I wrote in the previous paragraph is true, but I will do that in a separate PR after this one. The only thing we need gccWithoutTargetLibc to do is compile libgcc_s.so and glibc, and neither of those need libstdc++.

Getting gccWithoutTargetLibc to produce a working libgcc_s.so is actually pretty urgent (see #240484), so I don't want it to wait.

@ghost ghost marked this pull request as draft June 19, 2023 06:40
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 6.topic: rust 6.topic: steam 6.topic: vim 6.topic: vscode 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 28, 2023
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 28, 2023
@github-actions github-actions bot removed 6.topic: steam 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: documentation 6.topic: rust 6.topic: vim 8.has: changelog 6.topic: vscode labels Jun 28, 2023
@ghost ghost changed the title gccCrossStageStatic: enable dynamic libraries, rename to gccWithoutTargetLibc gccCrossStageStatic: enable dynamic libraries and rename it Jun 28, 2023
@ghost ghost changed the title gccCrossStageStatic: enable dynamic libraries and rename it gccCrossStageStatic: enable dynamic libraries, rename it Jun 28, 2023
@trofi
Copy link
Contributor

trofi commented Jul 13, 2023

Result of nixpkgs-review pr 238154 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
50 packages failed to build:
  • dxvk
  • dxvk.bin
  • dxvk.lib
  • exactaudiocopy
  • grapejuice
  • grapejuice.dist
  • klipper-firmware
  • klipper-flash
  • pipelight
  • pkgsLLVM.stdenv
  • playonlinux
  • q4wine
  • vkd3d
  • vkd3d-proton
  • winbox
  • wine (winePackages.full ,winePackages.stableFull)
  • wine-staging (winePackages.stagingFull)
  • wine-wayland (winePackages.waylandFull)
  • wine64 (wine64Packages.full ,wine64Packages.stableFull)
  • wine64Packages.base (wine64Packages.stable)
  • wine64Packages.staging
  • wine64Packages.stagingFull
  • wine64Packages.unstable
  • wine64Packages.unstableFull
  • wine64Packages.wayland
  • wine64Packages.waylandFull
  • winePackages.base (winePackages.stable)
  • winePackages.staging
  • winePackages.unstable
  • winePackages.unstableFull
  • winePackages.wayland
  • wineWow64Packages.base (wineWow64Packages.stable)
  • wineWow64Packages.full (wineWow64Packages.stableFull)
  • wineWow64Packages.staging
  • wineWow64Packages.stagingFull
  • wineWow64Packages.unstable
  • wineWow64Packages.unstableFull
  • wineWow64Packages.wayland
  • wineWow64Packages.waylandFull
  • wineWowPackages.base (wineWowPackages.stable)
  • wineWowPackages.full (wineWowPackages.stableFull)
  • wineWowPackages.staging
  • wineWowPackages.stagingFull
  • wineWowPackages.unstable
  • wineWowPackages.unstableFull
  • wineWowPackages.wayland
  • wineWowPackages.waylandFull
  • wineasio
  • yabridge
  • yabridgectl
10 packages built:
  • armTrustedFirmwareTools
  • fusee-launcher
  • gnuk
  • klipper-genconf
  • qmk
  • qmk.dist
  • simavr
  • spike
  • tests.makeBinaryWrapper
  • tinygo

@ghost
Copy link
Author

ghost commented Jul 13, 2023

Result of nixpkgs-review pr 238154 run on x86_64-linux 1

(Edit)

Most (maybe all) of this is fixed by these two PRs (mostly the second one):

(edit)

So this doesn't happen again:

@trofi
Copy link
Contributor

trofi commented Jul 14, 2023

Bisect claims that one of these commits also broke pkgsCross.m68k.stdenv in master:

$ nix build --no-link -f. pkgsCross.m68k.stdenv -L
...
m68k-unknown-linux-gnu-stage-static-gcc> make[1]: Leaving directory '/build/build/m68k-unknown-linux-gnu/libgcc'
m68k-unknown-linux-gnu-stage-static-gcc> Moving /nix/store/8658fc2zsh062ibn1l4vsvsapy4jppkz-m68k-unknown-linux-gnu-stage-static-gcc-12.3.0/m68k-unknown-linux-gnu/lib/libgcc_s.so to /nix/store/sf4g7jbpm502pp8lr2jh0qvxq2wqv0db-m68k-unknown-linux-gnu-stage-static-gcc-12.3.0-lib/m68k-unknown-linux-gnu/lib/libgcc_s.so
m68k-unknown-linux-gnu-stage-static-gcc> Moving /nix/store/8658fc2zsh062ibn1l4vsvsapy4jppkz-m68k-unknown-linux-gnu-stage-static-gcc-12.3.0/m68k-unknown-linux-gnu/lib/libgcc_s.so.2 to /nix/store/sf4g7jbpm502pp8lr2jh0qvxq2wqv0db-m68k-unknown-linux-gnu-stage-static-gcc-12.3.0-lib/m68k-unknown-linux-gnu/lib/libgcc_s.so.2
m68k-unknown-linux-gnu-stage-static-gcc> Removing empty /nix/store/8658fc2zsh062ibn1l4vsvsapy4jppkz-m68k-unknown-linux-gnu-stage-static-gcc-12.3.0/m68k-unknown-linux-gnu/lib/ and (possibly) its parents
m68k-unknown-linux-gnu-stage-static-gcc> /nix/store/qr60k7sz61hvv2baadlfv5qjr8q7vlg6-builder.sh: line 243: type: install_name_tool: not found
m68k-unknown-linux-gnu-stage-static-gcc> preFixupLibGccPhase
m68k-unknown-linux-gnu-stage-static-gcc> mv: cannot stat '/nix/store/sf4g7jbpm502pp8lr2jh0qvxq2wqv0db-m68k-unknown-linux-gnu-stage-static-gcc-12.3.0-lib/m68k-unknown-linux-gnu/lib/libgcc_s.so.1': No such file or directory

I think m68k defines libgcc_s.so.2, not libgcc_s.so.1.

@trofi
Copy link
Contributor

trofi commented Jul 14, 2023

And bisect claims that one of these commits also broke pkgsCross.s390.stdenv in master:

$ nix build --no-link -f. pkgsCross.s390.stdenv -L
...
s390-unknown-linux-gnu-stage-static-gcc> /build/build/./gcc/xgcc -B/build/build/./gcc/    -O2  -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE  -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition  -isystem ./include  -fPIC -mlong-double-128 -g -DIN_LIBGCC2 -fbuilding-libgcc -fno-stack-protector -Dinhibit_libc -fPIC -mlong-double-128 -I. -I. -I../.././gcc -I../../../gcc-12.3.0/libgcc -I../../../gcc-12.3.0/libgcc/. -I../../../gcc-12.3.0/libgcc/../gcc -I../../../gcc-12.3.0/libgcc/../include  -DHAVE_CC_TLS   -o morestack_s.o -MT morestack_s.o -MD -MP -MF morestack_s.dep -DSHARED -c -xassembler-with-cpp ../../../gcc-12.3.0/libgcc/config/s390/morestack.S
s390-unknown-linux-gnu-stage-static-gcc> make[1]: *** No rule to make target '../../../gcc-12.3.0/libgcc/config/s390/crti.S', needed by 'crti.o'.  Stop.
s390-unknown-linux-gnu-stage-static-gcc> make[1]: *** Waiting for unfinished jobs....
s390-unknown-linux-gnu-stage-static-gcc> /nix/store/51sszqz1d9kpx480scb1vllc00kxlx79-bash-5.2-p15/bin/bash ../../../gcc-12.3.0/libgcc/../move-if-change tmp-libgcc_tm.h libgcc_tm.h
s390-unknown-linux-gnu-stage-static-gcc> echo timestamp > libgcc_tm.stamp
s390-unknown-linux-gnu-stage-static-gcc> ../../../gcc-12.3.0/libgcc/config/s390/morestack.S: Assembler messages:
s390-unknown-linux-gnu-stage-static-gcc> ../../../gcc-12.3.0/libgcc/config/s390/morestack.S:600: Warning: ignoring incorrect section type for .init_array.00000
s390-unknown-linux-gnu-stage-static-gcc> make[1]: Leaving directory '/build/build/s390-unknown-linux-gnu/libgcc'
s390-unknown-linux-gnu-stage-static-gcc> make: *** [Makefile:12947: all-target-libgcc] Error 2
error: builder for '/nix/store/lffjd90i8pa7mbwjcxpwf8cxaa2xqljp-s390-unknown-linux-gnu-stage-static-gcc-12.3.0.drv' failed with exit code 2;

Don't see why immediately.

@ghost
Copy link
Author

ghost commented Jul 15, 2023

Bisect claims that one of these commits also broke pkgsCross.m68k.stdenv in master:

And bisect claims that one of these commits also broke pkgsCross.s390.stdenv in master:

Please add these to tests.cross.sanity so they are tested for; if there are no tests for something, it is certain to break.

Also, I'm pretty sure none of us can afford S390 hardware.

@trofi
Copy link
Contributor

trofi commented Jul 15, 2023

$ nix build -f.  tests.cross.sanity
error: attribute 'sanity' in selection path 'tests.cross.sanity' not found

@ghost
Copy link
Author

ghost commented Jul 15, 2023

@ofborg build tests.cross.sanity

@trofi
Copy link
Contributor

trofi commented Jul 17, 2023

Added assert against an attrset field caused the non-determinism regression: #244045 (a patch for nix to ease reproducibility: NixOS/nix#8711)

@trofi
Copy link
Contributor

trofi commented Sep 1, 2023

Another regression is pkgCross.loongarch64-linux.stdenv reported by jiegec: #252590

@mikatammi
Copy link
Contributor

I'm guessing the automatic backport failed and we never got backport to 23.05? Could this be backported, it would be nice, as I'm seeing the cross-compiled Wayland failing on runtime, and this PR fixes the issue

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

Backport failed for release-23.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-23.05
git worktree add -d .worktree/backport-238154-to-release-23.05 origin/release-23.05
cd .worktree/backport-238154-to-release-23.05
git checkout -b backport-238154-to-release-23.05
ancref=$(git merge-base 931b8a4979df477c0baffc2544f593d7a9b32dcb 96a2f1b4e1315251f1916f64c4632dbf7eb40522)
git cherry-pick -x $ancref..96a2f1b4e1315251f1916f64c4632dbf7eb40522

@alyssais
Copy link
Member

alyssais commented Oct 2, 2023

This PR caused some regressions, so any potential backport should probably be okayed by the release managers before being merged.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
8 participants